-
Notifications
You must be signed in to change notification settings - Fork 805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow path in from_py_with
and deprecate string literal
#4860
Conversation
5a5e09a
to
8fdecc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry for the slow review. I should be able to reliably be more active again now.
LGTM with one small suggestion before merge...
quote_spanned! { expr_path.span() => | ||
#[deprecated(since = "0.24.0", note = "`from_py_with` string literals are deprecated. Use the function path instead.")] | ||
#[allow(dead_code)] | ||
const LIT_STR_DEPRECATION: () = (); | ||
let _: () = LIT_STR_DEPRECATION; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid repetition, shall we put a helper function somewhere to produce these fragments?
Also, I wonder if we should be more direct with the hint to users. E.g.
= help: use `foo` instead of `"foo"`
(we could even substitute in the user input)
or maybe
remove the quotes from the literal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I made a small helper in utils
and changed the deprecation message, let me know what you think
warning: use of deprecated constant `__pyfunction_f::LIT_STR_DEPRECATION`: `from_py_with` string literals is deprecated. Use the function path instead. | ||
--> tests/ui/invalid_argument_attributes.rs:23:28 | ||
| | ||
23 | fn f(#[pyo3(from_py_with = "bytes_from_py")] _bytes: Vec<u8>) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so e.g. maybe here it says
use `bytes_from_py` instead of `"bytes_from_py"`
9851988
to
d937436
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, thanks!
This allows specifying the
from_py_with
function as a path rather than a string literal. The old syntax is still valid and shows a deprecation warning.Complementary to #4850.